feat(erc1155): add custom ERC1155 contract for airdrop #262

dohaki · 2023-03-20T14:18:32Z

Part of ACX-950

I also deployed to Mumbai Testnet here https://mumbai.polygonscan.com/address/0x17496824ba574a4e9de80110a91207c4c63e552a and tested the integration with OpenSea here https://testnets.opensea.io/assets/mumbai/0x17496824ba574a4e9de80110a91207c4c63e552a/0.

But I wasn't able to verify the contract via Polyscan though 🤔 When I run

yarn hardhat etherscan-verify --network polygon-mumbai --license AGPL-3.0 --force-license --solc-input

with the env var POLYGON_ETHERSCAN_API_KEY being set, I get

verifying MintableERC1155 (0x17496824Ba574A4e9De80110A91207c4c63e552a) ...
contract MintableERC1155 failed to submit : "NOTOK" : "Invalid API Key" [object Object]
already verified: PolygonTokenBridger (0x97f102f2f73717e203f964Ad9940e4C2e79b8597), skipping.
already verified: Polygon_SpokePool (0x45fF03629D024b7763275e732a2d80202c18b31C), skipping.

contracts/erc1155/MintableERC1155.sol

nicholaspai

This is awesome @dohaki and the scripts + tests look great. My main recommendation is to remove the AccessControlEnumerable and just replace it with a simple Ownable. We can have the owner have minting, pausing, and setting URI ability. This will keep the contract simpler.

My other suggestion is to add NatSpec comments to all Solidity files. This will importantly render the function comments and descriptions on Etherscan! GitHub CoPilo is great for this ;)

james-a-morris

Left a few comments. Looks amazing so far, @dohaki

contracts/erc1155/MintableERC1155.sol

mrice32 · 2023-03-20T15:48:09Z

contracts/erc1155/MintableERC1155.sol

+    function setTokenURI(uint256 tokenId, string memory tokenURI) external onlyOwner {
+        _tokenURIs[tokenId] = tokenURI;
+    }


Do we want it to be possible to reset a token URI after it's already been set?

hm I guess not actually, it would give holders more confidence. So maybe require existing tokenURI is empty

Added a require here 898de67

contracts/erc1155/MintableERC1155.sol

nicholaspai · 2023-03-20T15:49:44Z

contracts/erc1155/MintableERC1155.sol

+     * @param tokenId Token type to airdrop.
+     * @param amount Amount of token types to airdrop.
+     *
+     * NOTE: Call might run out of gas if `recipients` arg too long. Might need to chunk up the list.


Would just make this a @dev call instead of NOTE

Done 898de67

nicholaspai · 2023-03-20T15:50:12Z

contracts/erc1155/MintableERC1155.sol

+    constructor() ERC1155("") {}
+
+    /**
+     * @dev Creates `amount` new tokens for `recipients` of token type `tokenId`.


Nit, explanation of function should be @notice not @dev. Use @dev for caller warnings like the out of gas limit

nicholaspai · 2023-03-20T15:51:43Z

contracts/erc1155/MintableERC1155.sol

+     * @param tokenURI URI of token metadata.
+     */
+    function setTokenURI(uint256 tokenId, string memory tokenURI) external onlyOwner {
+        _tokenURIs[tokenId] = tokenURI;


nit: emit an Event after changing state. Something like SetTokenURI(uint256 tokenId, string newUri)

Yes good point. Actually in the spec we should emit URI(uri, tokenId). See https://eips.ethereum.org/EIPS/eip-1155#specification

nicholaspai · 2023-03-20T15:52:07Z

contracts/erc1155/MintableERC1155.sol

+     * `setTokenURI` to allow IPFS URIs for all token types.
+     * @param tokenId Token type to retrieve metadata URI for.
+     */
+    function uri(uint256 tokenId) public view virtual override returns (string memory) {


nit: no need to make this virtual anymore

nicholaspai · 2023-03-20T15:54:27Z

contracts/erc1155/MintableERC1155.sol

+    }
+
+    /**
+     * @dev Instead of returning the same URI for *all* token types, we return the uri set by


nit: Add a @notice explaining this function. Can use @inheritdoc ERC1155 for example

nicholaspai · 2023-03-20T15:54:37Z

contracts/erc1155/MintableERC1155.sol

+    }
+
+    /**
+     * @dev Sets the URI for token of type `tokenId` to `tokenURI`.


nit: make @notice

nicholaspai

Looks great! I left some nits

- emit URI event - require token uri not set - doc improvements

nicholaspai · 2023-03-20T16:20:32Z

contracts/erc1155/MintableERC1155.sol

+import "@openzeppelin/contracts/access/Ownable.sol";
+import "@openzeppelin/contracts/token/ERC1155/ERC1155.sol";
+
+contract MintableERC1155 is ERC1155, Ownable {


nit: add a @title, @notice comment above the Contract. Something like:

@title MintableERC1155 @notice Ownable contract enabling owner to airdrop many recipients the same token ID at once

nicholaspai · 2023-03-20T16:20:47Z

contracts/erc1155/MintableERC1155.sol

+import "@openzeppelin/contracts/token/ERC1155/ERC1155.sol";
+
+contract MintableERC1155 is ERC1155, Ownable {
+    mapping(uint256 => string) public _tokenURIs;


should add comment above all state vars including _tokenURIs

nicholaspai · 2023-03-20T16:21:20Z

contracts/erc1155/MintableERC1155.sol

+    event Airdrop(address caller, uint256 tokenId, address[] recipients, uint256 amount);
+
+    // solhint-disable-next-line
+    constructor() ERC1155("") {}


I might add a comment why you're passing in "" (because we opt to use the _tokenURIs mapping instead)

amateima · 2023-03-20T16:25:08Z

scripts/mintERC1155.ts

+      airdropTx.hash
+    );
+    await airdropTx.wait();
+    console.log(`Successfully minted token to:`, recipientsChunk);


Would make sense to set the token URI here too or we'll submit a manual tx for this?

I wanted to create a separate script in a separate PR that does this. Basically

Parse and validate metadata

Upload to IPFS

Set token URI

mrice32

LGTM!

feat(erc1155): add custom ERC1155 contract for airdrop

5f9d572

dohaki requested review from amateima, james-a-morris, mrice32 and nicholaspai March 20, 2023 14:18

nicholaspai reviewed Mar 20, 2023

View reviewed changes

contracts/erc1155/MintableERC1155.sol Outdated Show resolved Hide resolved

nicholaspai reviewed Mar 20, 2023

View reviewed changes

contracts/erc1155/MintableERC1155.sol Outdated Show resolved Hide resolved

nicholaspai reviewed Mar 20, 2023

View reviewed changes

contracts/erc1155/MintableERC1155.sol Outdated Show resolved Hide resolved

nicholaspai reviewed Mar 20, 2023

View reviewed changes

james-a-morris reviewed Mar 20, 2023

View reviewed changes

review requests

b5831e7

dohaki requested review from nicholaspai and james-a-morris March 20, 2023 15:48

mrice32 reviewed Mar 20, 2023

View reviewed changes

nicholaspai reviewed Mar 20, 2023

View reviewed changes

contracts/erc1155/MintableERC1155.sol Outdated Show resolved Hide resolved

nicholaspai reviewed Mar 20, 2023

View reviewed changes

contracts/erc1155/MintableERC1155.sol Outdated Show resolved Hide resolved

nicholaspai reviewed Mar 20, 2023

View reviewed changes

nicholaspai approved these changes Mar 20, 2023

View reviewed changes

dohaki added 3 commits March 20, 2023 17:04

refactor: review request

898de67

- emit URI event - require token uri not set - doc improvements

update abis

bcc161f

improve test

31eb7fa

dohaki requested a review from mrice32 March 20, 2023 16:11

nicholaspai reviewed Mar 20, 2023

View reviewed changes

nicholaspai approved these changes Mar 20, 2023

View reviewed changes

amateima approved these changes Mar 20, 2023

View reviewed changes

docs: improve inline comments

d7538d6

james-a-morris approved these changes Mar 20, 2023

View reviewed changes

mrice32 approved these changes Mar 20, 2023

View reviewed changes

dohaki merged commit 7acc87e into master Mar 21, 2023

dohaki deleted the erc1155 branch March 21, 2023 08:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(erc1155): add custom ERC1155 contract for airdrop #262

feat(erc1155): add custom ERC1155 contract for airdrop #262

dohaki commented Mar 20, 2023

nicholaspai left a comment

james-a-morris left a comment

mrice32 Mar 20, 2023

nicholaspai Mar 20, 2023

dohaki Mar 20, 2023

nicholaspai Mar 20, 2023

dohaki Mar 20, 2023

nicholaspai Mar 20, 2023

nicholaspai Mar 20, 2023

dohaki Mar 20, 2023

nicholaspai Mar 20, 2023

nicholaspai Mar 20, 2023

nicholaspai Mar 20, 2023

nicholaspai Mar 20, 2023

nicholaspai left a comment

nicholaspai Mar 20, 2023

nicholaspai Mar 20, 2023

nicholaspai Mar 20, 2023

amateima Mar 20, 2023 •

edited

Loading

dohaki Mar 20, 2023

amateima Mar 20, 2023

mrice32 left a comment

feat(erc1155): add custom ERC1155 contract for airdrop #262

feat(erc1155): add custom ERC1155 contract for airdrop #262

Conversation

dohaki commented Mar 20, 2023

nicholaspai left a comment

Choose a reason for hiding this comment

james-a-morris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicholaspai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amateima Mar 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrice32 left a comment

Choose a reason for hiding this comment

amateima Mar 20, 2023 •

edited

Loading